[Issue 2247] fix datatype of vhost:headers from String to Array #2248
Conversation
Fix wrong introduction of Dataype String to headers parameter The code requires the parameter to be Array[String] to work properly
|
|
|
From what I can see this looks to be correct, but could you also update the example in the description so that it matches |
|
@david22swan thanks for the review, description is updated |
|
@chillinger Look's like your getting some unit test failures. |
| @@ -1853,7 +1853,7 @@ | |||
| Optional[Variant[Array[String],String]] $redirectmatch_status = undef, | |||
| Optional[Variant[Array[String],String]] $redirectmatch_regexp = undef, | |||
| Optional[Variant[Array[String],String]] $redirectmatch_dest = undef, | |||
| Optional[String] $headers = undef, | |||
| Optional[Array[String]] $headers = undef, | |||
There was a problem hiding this comment.
If you're changing it to an array, I'd say drop the Optional part and make it an empty array by default. I'd also narrow it to non-empty strings:
| Optional[Array[String]] $headers = undef, | |
| Array[String[1]] $headers = [], |
Then you can simplify templates/vhost/_header.erb a lot too.
You can also drop the if $headers and stick to ! empty($headers) on line 2263.
There was a problem hiding this comment.
@ekohl I thought about that, but realised the patch the way I did for the following reasons:
- I only have the time to actually fix the bug which breaks our puppet runs, I did not want to rework more than really necessary
- every other array type parameter in the module, including $request_headers is an Optional[Array[String]]. I wanted to stay consistent
For those reasons I would like this fix to be accepted. If the datatype should be changed to Array with default [], I would like this seen as an independent rework issue
There was a problem hiding this comment.
I do think it should be fixed so I opened #2251.
|
@chillinger Apologies, but after some discussion within the team we have decided to go ahead with the changes proposed by @ekohl rather than your own, as they are more comprehensive and represent a larger improvement to the module. |
|
@david22swan @ekohl no problem at all, I'm happy an even better solution arose from my initial proposal! |
|
@chillinger Closing this as Ekohl's pr has been merged. |
Fix wrong introduction of Dataype String to headers parameter
The code requires the parameter to be Array[String] to work properly